Skip to content

Conversation

@pat-s
Copy link
Contributor

@pat-s pat-s commented Oct 24, 2025

This is currently broken (and according to my backup history since ~ 2 months).

With the following config

pgbackrest_install: true
pgbackrest_repo_type: 's3'
pgbackrest_archive_command: 'pgbackrest --stanza={{ pgbackrest_stanza }} archive-push %p'
[...]

the bootstrap.dcs.postgresql.parameters.archive_command option in patroni.yml was still set to the default cd .. This caused all pgbackrest cron runs to fail.

Maybe there is a better way to fix this, but the following patch works.

@vitabaks
Copy link
Owner

it is assumed that the user will specify the desired archive_command in the postgresql_parameters variable, example:

postgresql_parameters:
  - { option: "archive_command", value: "{{ pgbackrest_archive_command }}" }
  - ...

But if he didn't do it explicitly, then we substitute him at the pre-checks level - https://github.com/vitabaks/autobase/blob/2.4.1/automation/roles/pre_checks/tasks/pgbackrest.yml

---
# Patroni configure

# Ensure archive_command is set correctly for pgbackrest (same logic as pre_checks/pgbackrest.yml)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why we should duplicate the logic here, because we are already set archive_command in pre_checks role.

@pat-s
Copy link
Contributor Author

pat-s commented Oct 25, 2025

it is assumed that the user will specify the desired archive_command in the postgresql_parameters variable

Ah! Though I am not sure if this is best approach:

  • Doing so is non-intuitive, especially if there are dedicated pgbackrest_* vars available including the pgbackrest_archive_command
  • pgbackrest_archive_command seems to be a no-op right now. If it shouldn't be used at all, it should be removed.

I personally try to avoid setting postgresql_parameters as it requires adding the full default definition into my group_vars, as otherwise these params aren't set anymore. However, this also means I can't easily spot anymore which of these I've now actually changed and which ones are defaults. This is a known issue with (long) variable list definitions in Ansible.

From a user PoV, I'd highly prefer pgbackrest_archive_command being respected (if set). This is what this PR is doing.
I see it duplicates code in some ways but in the end it just ensure a proper use of the var in the first place. Maybe there are better ways but I don't think the current way is a good approach in the longterm.

@vitabaks
Copy link
Owner

I'm thinking of adding a postgresql_archive_command variable that will dynamically substitute a value based on which backup tool is being used, and use this variable as the value of the archive_command option.

@vitabaks vitabaks marked this pull request as draft October 25, 2025 15:03
@pat-s
Copy link
Contributor Author

pat-s commented Oct 25, 2025

That might be an option yes. From a user PoV, I'd be happy not having to set postgresql_archive_command or pgbackrest_archive_command unless I really want to put something modified there.

The best would be to have a default for backups in place and even when deciding for another tool, only having to use <tool name>_enabled: true or similar to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants